Do these work with both runc and crun?

@mheon

Do these work with both runc and crun?

Yes, it should work the same with both.

[root@lima-default]~# podman container restore server
Error: OCI runtime error: checkpoint contains established TCP connections, restore requires --tcp-established or --tcp-close

[root@lima-default]~# podman info --format '{{.Host.OCIRuntime.Name}}'
runc

It's a CRIU error being matched.

@Luap99

that does not seem maintainable, we should not make assumptions about the error text from crun or criu, it changes often enough. Sure the tests depend on it to some extend but we should not make the code depend, i.e. this may not work for another runtime.

It would be better to work with crun and criu to produce better errors to begin with.

I agree, but that's exactly what caused this bug report.
Not only the tests depend on the error output, but there's getOCIRuntimeError() that tries to match the error text and return a corresponding Podman error based on that. Because of changes in the crun output, the cg: cgroupd: recv req error: No such file or directory error was incorrectly converted to OCI runtime attempted to invoke a command that was not found by this regex and the fact that other error lines from CRIU were swallowed by the buggy JSON Unmarshal not handling multiple objects:

if match := regexp.MustCompile("(?i).*executable file not found in.*|.*no such file or directory.*|.*open executable.*").FindString(runtimeMsg); match != "" {
	errStr := match
	if includeFullOutput {
		errStr = runtimeMsg
	}
	return fmt.Errorf("%s: %s: %w", name, strings.Trim(errStr, "\n"), define.ErrOCIRuntimeNotFound)
}

Unfortunately, there's AFAIK no specific error code returned by crun or CRIU in this case that could be used and that might not be stable either.

Alternatively, the whole error output from crun could be shown to the user, or there can be no change at all - it still works as documented.

But in my opinion and from the perspective of a user, I'd like to see a helpful error message in this case, even though it's documented in https://docs.podman.io/en/latest/markdown/podman-container-restore.1.html#tcp-established

Not only the tests depend on the error output, but there's getOCIRuntimeError() that tries to match the error text and return a corresponding Podman

which I absolutely hate, the fact we eat up errors and convert them often also creates just more confusing errors as well. It also often has no way of working with multiple different runtimes.

Alternatively, the whole error output from crun could be shown to the user, or there can be no change at all - it still works as documented.

I think parsing the full error log is the right thing to do, if it gives us many lines we should forward all of them

@Luap99

Not only the tests depend on the error output, but there's getOCIRuntimeError() that tries to match the error text and return a corresponding Podman

which I absolutely hate, the fact we eat up errors and convert them often also creates just more confusing errors as well. It also often has no way of working with multiple different runtimes.

Alternatively, the whole error output from crun could be shown to the user, or there can be no change at all - it still works as documented.

I think parsing the full error log is the right thing to do, if it gives us many lines we should forward all of them

Thank you. Do you mean getting rid of the whole getOCIRuntimeError mechanism and just forwarding the received error output with some generic error? Because I'm afraid it's not possible to limit the change to just the case in this bug without relying on the crun/CRIU error message text.

If so, I think it's quite a big and possibly breaking change in the error messages that would users receive.

Anyway, I'd still like finding a way of providing some context to users (in this case guiding them to use the --tcp-established or --tcp-close

Thank you. Do you mean getting rid of the whole getOCIRuntimeError mechanism and just forwarding the received error output with some generic error? Because I'm afraid it's not possible to limit the change to just the case in this bug without relying on the crun/CRIU error message text.

I would love to go into that direction.

At the minimum I think we should always forward all error messages, i.e. just use the parseOCIErrors() for all messages. We can return a multi line error with errors.Join() for all of them.

If so, I think it's quite a big and possibly breaking change in the error messages that would users receive.

Well error messages should provide the proper context what went wrong, they are not a stable API by design.
There is some special notes here with the 126/127 exit codes we provide for certain errors.

Anyway, I'd still like finding a way of providing some context to users (in this case guiding them to use the --tcp-established or --tcp-close

I don't strictly disagree but overall I find most of these assumption just do not hold long term.
Connected TCP socket in image is a hard coded assumption about a error string from criu, they can just that or maybe the error test could mean something else.

If we want better errors I would argue this needs to be pushed back to criu. They know whenever the tcp-established option is set or not, so they could just as well change the message to Connected TCP socket in image but neither the closing nor preserving of tcp sockets was requested which would help a lot already.

Also the other problem with this I did not even touch yet is that we really should not hard code cli flags in error message in the backend code. We have a fully support REST API, returning cli arguments to some user who is not using the cli is not useful either.